SPLAT-2167: Added dedicated host support for AWS #374
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@vr4manta: This pull request references SPLAT-2167 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughWalkthroughAdds AWS dedicated-host placement support across CAPA↔MAPI conversions (HostAffinity/HostID mapping and validation, tenancy constants), updates fuzz and unit tests for many placement scenarios, bumps several module dependencies, and adds ec2:DescribeInstanceTypes to the AWS credentials-request manifest. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
openshift-merge-robot
added
the
needs-rebase
|
@vr4manta: This pull request references SPLAT-2167 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-merge-robot
removed
the
needs-rebase
vr4manta
force-pushed
the
SPLAT-2167
branch
2 times, most recently
from
6355141 to
7ba97c3
Compare
|
@vr4manta: This pull request references SPLAT-2167 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
vr4manta
force-pushed
the
SPLAT-2167
branch
2 times, most recently
from
dc53d82 to
e5cbce2
Compare
|
/test all |
|
/test all |
vr4manta
force-pushed
the
SPLAT-2167
branch
2 times, most recently
from
ed41da7 to
320eac0
Compare
|
@vr4manta: This pull request references SPLAT-2167 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
| - ec2:DescribeDhcpOptions | ||
| - ec2:DescribeImages | ||
| - ec2:DescribeInstances | ||
| - ec2:DescribeInstanceTypes |
There was a problem hiding this comment.
I believe it would be nice to add this change to a new commit referencing the reason of adding it (upstream PR, jira feature, etc).
this would be used to document in the future and/or in managed services to eventually update the managed IAM.
There was a problem hiding this comment.
Sure! There is a warning message that is caused by this missing. I am not 100% sure this feature introduced it, but I am happy to put into a separate commit.
pkg/conversion/capi2mapi/aws.go
Outdated
| mapaProviderConfig.HostPlacement = &mapiv1beta1.HostPlacement{ | ||
| Affinity: ptr.To(mapiv1beta1.HostAffinityDedicatedHost), | ||
| DedicatedHost: &mapiv1beta1.DedicatedHost{ | ||
| ID: *m.awsMachine.Spec.HostID, |
There was a problem hiding this comment.
do we need to check HostID isnt nil to be safe or is there already a previous validation of this scenario?
There was a problem hiding this comment.
good question. lots of these CRs have checks in their respective webhooks / CRDs. I can add a check here as well. I was on the fence about it.
|
@vr4manta: This pull request references SPLAT-2167 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
openshift-merge-robot
removed
the
needs-rebase
This is happening because upstream bug defaults to host, not default. this results in the conversion logic failing hostID for not being set when in reality it should not be due to default. |
|
@vr4manta: This pull request references SPLAT-2167 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @e2e/go.mod:
- Line 27: Add a TODO comment above the pseudo-version dependency line for
sigs.k8s.io/cluster-api-provider-aws/v2 v2.10.1-0.20251223195230-7aa98eaf0ba3
noting this is a temporary workaround that depends on an upstream PR fix
(include PR number/link and an expected removal condition), mirroring the style
and placement used for other temporary dependencies in lines 5-10 so future
maintainers know why this pseudo-version is vendored and when to revert it.
In @hack/tools/go.mod:
- Line 11: Your updated dependency line "sigs.k8s.io/kustomize/kustomize/v5
v5.7.1" introduces breaking changes: v5.5.0 removed Starlark KRM function
support and v5.7.1 changed exec plugin arg parsing; inspect any use of
Starlark-based KRM functions and any exec plugins in your kustomize
overlays/plugins, run end-to-end builds to validate behavior, and either (a)
adapt those plugins/manifests to native Go functions or alternative scripting or
adjust exec plugin invocation to the new argument parsing, or (b) pin the
kustomize version to a compatible release (e.g., v5.4.1) in the go.mod until
you’ve migrated and tested.
In @pkg/conversion/capi2mapi/aws.go:
- Around line 258-260: The branch that validates HostID format currently appends
errHostIDRequired when awsDedicatedHostNamePattern.MatchString(*spec.HostID)
fails; change that to append errHostIDInvalidFormat instead so the error
reflects an invalid format (use the same error symbol as the default case),
i.e., replace errHostIDRequired with errHostIDInvalidFormat in the errorList
append for the awsDedicatedHostNamePattern check involving spec.HostID and
fldPath.Child("dedicatedHost").Child("id").
🧹 Nitpick comments (4)
go.mod (1)
318-320: Remove or clarify the confusing trailing comments.The comment states "Using replace for force update" but the actual implementation uses a
requiredirective (line 41), not areplacedirective. The commented-outreplacedirective is redundant and adds noise.Consider either:
- Removing these trailing lines entirely since the version is already specified at line 41
- Clarifying the comment to explain why this is documented (e.g., "Note: Previously considered using replace directive, but using require with pseudo-version instead")
♻️ Proposed cleanup
sigs.k8s.io/structured-merge-diff/v6 v6.3.0 // indirect ) - -// Using replace for force update of 2.10 -//replace sigs.k8s.io/cluster-api-provider-aws/v2 => sigs.k8s.io/cluster-api-provider-aws/v2 v2.10.1-0.20251223195230-7aa98eaf0ba3pkg/conversion/capi2mapi/aws.go (2)
176-176: Stale TODO comment.This TODO indicates conversion should be added when the MAPI API is finalized, but the conversion is now implemented at lines 187-191 directly below. Consider removing or updating this comment.
Proposed fix
- // HostPlacement: TODO: add conversion from CAPA HostAffinity and HostID to MAPI HostPlacement when the MAPI API is finalized.
252-285: Consider using defined constants instead of string literals.The switch cases use string literals
"host"and"default", but constantsTenancyHostandTenancyDefaultare defined in this same file (lines 59, 63). Using the constants would improve maintainability.Proposed fix
switch *spec.HostAffinity { - case "host": + case TenancyHost: // For "host", host id is required in mapi. Let's make sure it is set and id is valid if spec.HostID == nil { errorList = append(errorList, field.Required(fldPath.Child("dedicatedHost").Child("id"), errHostIDRequired)) break } else if !awsDedicatedHostNamePattern.MatchString(*spec.HostID) { - errorList = append(errorList, field.Invalid(fldPath.Child("dedicatedHost").Child("id"), *spec.HostID, errHostIDRequired)) + errorList = append(errorList, field.Invalid(fldPath.Child("dedicatedHost").Child("id"), *spec.HostID, errHostIDInvalidFormat)) break } host = &mapiv1beta1.HostPlacement{ Affinity: ptr.To(mapiv1beta1.HostAffinityDedicatedHost), DedicatedHost: &mapiv1beta1.DedicatedHost{ ID: *spec.HostID, }, } - case "default": + case TenancyDefault: host = &mapiv1beta1.HostPlacement{ Affinity: ptr.To(mapiv1beta1.HostAffinityAnyAvailable), }pkg/conversion/mapi2capi/aws.go (1)
276-279: Stale TODO comments.These TODO comments indicate the conversion is not yet implemented, but lines 294-295 now set
HostAffinityandHostIDusing the new helper functions. Consider removing these stale comments.Proposed fix
- // TODO: add conversion from MAPI HostPlacement to CAPA HostAffinity and HostID when the MAPA API is finalized. - // HostAffinity: not set yet as they are not stable in MAPI yet. - // HostID: not set yet as they are not stable in MAPI yet. -
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (44)
e2e/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumhack/tools/go.sumis excluded by!**/*.sumvendor/github.com/sergi/go-diff/AUTHORSis excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/CONTRIBUTORSis excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/diff.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/diffmatchpatch.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/match.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/mathutil.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/operation_string.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/patch.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/stringutil.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/awsmachine_types.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/awsmachine_webhook.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/awsmachinetemplate_webhook.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/types.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/cmd/config/internal/commands/grep.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/cmd/config/internal/commands/run-fns.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/cmd/config/internal/generateddocs/api/docs.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/build.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/flagenablehelm.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/flagsforfunctions.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/create/create.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addbase.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addbuildmetadata.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addcomponent.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addgenerator.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addmetadata.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addresource.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addtransformer.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/fix/convert.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removebuildmetadata.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removeconfigmap.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removemetadata.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removeresource.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removesecret.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removetransformer.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/internal/kustfile/kustomizationfile.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/internal/util/validate.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/localize/localize.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kyaml/copyutil/copyutil.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (10)
e2e/go.modgo.modhack/tools/go.modmanifests/0000_30_cluster-api_01_credentials-request.yamlpkg/conversion/capi2mapi/aws.gopkg/conversion/capi2mapi/aws_fuzz_test.gopkg/conversion/capi2mapi/aws_test.gopkg/conversion/mapi2capi/aws.gopkg/conversion/mapi2capi/aws_fuzz_test.gopkg/conversion/mapi2capi/aws_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/conversion/mapi2capi/aws_test.go
- manifests/0000_30_cluster-api_01_credentials-request.yaml
- pkg/conversion/mapi2capi/aws_fuzz_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/conversion/capi2mapi/aws_fuzz_test.go (1)
pkg/conversion/capi2mapi/aws.go (3)
TenancyDefault(59-59)TenancyDedicated(61-61)TenancyHost(63-63)
🔇 Additional comments (8)
go.mod (1)
41-41: Verify the AWS provider pseudo-version contains expected upstream fixes and evaluate the TODO comment necessity.The pseudo-version
v2.10.1-0.20251223195230-7aa98eaf0ba3should be confirmed to include the dedicated host fixes mentioned in PR objectives and validated for consistency across allgo.modfiles. The suggested TODO comment referencing issue #5801 requires verification that this PR reference is accurate and that a temporary marker is appropriate for this dependency.pkg/conversion/capi2mapi/aws.go (1)
187-191: LGTM!The dedicated host conversion is correctly integrated into
toProviderSpec. Errors are properly aggregated and the placement host field is set on the provider config.pkg/conversion/capi2mapi/aws_test.go (2)
74-117: LGTM!Comprehensive test coverage for valid HostAffinity and HostID combinations, including both 8-character and 17-character host IDs for both "default" and "host" affinity types.
118-183: LGTM!Thorough negative test cases covering various invalid HostID formats: too short, wrong lengths (9 and 16 characters), uppercase characters, missing
h-prefix, and non-hex characters. This ensures the regex validation is properly tested.pkg/conversion/capi2mapi/aws_fuzz_test.go (1)
196-223: LGTM!The fuzzer function is well-structured with 6 cases covering the valid combinations of Tenancy, HostAffinity, and HostID. The hardcoded HostID
"h-0123456789abcdef0"is a valid 17-character hex format. Good coverage for round-trip fuzz testing.pkg/conversion/mapi2capi/aws.go (3)
501-516: LGTM!The function correctly handles nil-safety for both
placementandplacement.Affinity. The mapping from MAPIHostAffinityAnyAvailable→"default"andHostAffinityDedicatedHost→"host"is consistent with the reverse conversion incapi2mapi/aws.go.
518-531: LGTM!The function correctly returns the HostID only when appropriate (for
AnyAvailableorDedicatedHostaffinity) and safely handles nilDedicatedHost. This aligns with the symmetric conversion behavior incapi2mapi.
294-295: LGTM!The new fields are correctly populated using the conversion helpers. This completes the bidirectional conversion support for dedicated host placement.
| k8s.io/utils v0.0.0-20250820121507-0af2bda4dd1d | ||
| sigs.k8s.io/cluster-api v1.11.3 | ||
| sigs.k8s.io/cluster-api-provider-aws/v2 v2.10.0 | ||
| sigs.k8s.io/cluster-api-provider-aws/v2 v2.10.1-0.20251223195230-7aa98eaf0ba3 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the full e2e/go.mod file to see the context
cat e2e/go.modRepository: openshift/cluster-capi-operator
Length of output: 7048
🏁 Script executed:
# Search for the commit hash and PR references in the codebase
rg "7aa98eaf0ba3|5801" -C 3Repository: openshift/cluster-capi-operator
Length of output: 29898
Add TODO comment for consistency with other temporary dependencies.
This pseudo-version should include a TODO comment documenting that it depends on an upstream PR fix, following the pattern used for other temporary dependencies in lines 5-10. The module is properly vendored, but lacks documentation explaining its temporary status.
Suggested change
+// TODO: Update to released version once upstream fix is available
sigs.k8s.io/cluster-api-provider-aws/v2 v2.10.1-0.20251223195230-7aa98eaf0ba3📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sigs.k8s.io/cluster-api-provider-aws/v2 v2.10.1-0.20251223195230-7aa98eaf0ba3 | |
| // TODO: Update to released version once upstream fix is available | |
| sigs.k8s.io/cluster-api-provider-aws/v2 v2.10.1-0.20251223195230-7aa98eaf0ba3 |
🤖 Prompt for AI Agents
In @e2e/go.mod at line 27, Add a TODO comment above the pseudo-version
dependency line for sigs.k8s.io/cluster-api-provider-aws/v2
v2.10.1-0.20251223195230-7aa98eaf0ba3 noting this is a temporary workaround that
depends on an upstream PR fix (include PR number/link and an expected removal
condition), mirroring the style and placement used for other temporary
dependencies in lines 5-10 so future maintainers know why this pseudo-version is
vendored and when to revert it.
| github.com/jstemmer/go-junit-report v0.9.1 | ||
| github.com/onsi/ginkgo/v2 v2.27.2 | ||
| sigs.k8s.io/kustomize/kustomize/v5 v5.4.1 | ||
| sigs.k8s.io/kustomize/kustomize/v5 v5.7.1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the current state of the go.mod file to see the actual versions
cat hack/tools/go.mod | head -30Repository: openshift/cluster-capi-operator
Length of output: 206
🌐 Web query:
kustomize v5.4.1 v5.7.1 breaking changes release notes
💡 Result:
Summary of breaking changes between kustomize v5.4.1 and v5.7.1:
- v5.4.0 (fixed in v5.4.1): null YAML values were being replaced by the string "null" (breaking change introduced in v5.4.0). [1]. (newreleases.io)
- v5.5.0: removal of Starlark support for KRM functions (breaking change). [2]. (newreleases.io)
- v5.7.1: no new breaking behavior called out in the v5.7.1 release notes; the release primarily drops the shlex dependency and updates libraries (note: if your workflows rely on the exec plugin argument parsing, test after upgrading). [3]. (github.com)
Sources:
[1] kustomize v5.4.1 / v5.4.0 release notes. (newreleases.io)
[2] kustomize v5.5.0 release notes (breaking: Starlark support removed). (newreleases.io)
[3] kustomize v5.7.1 release notes (drop shlex dependency; dependency updates). (github.com)
Would you like links to the specific GitHub release pages or a short upgrade checklist?
Citations:
- 1: https://newreleases.io/project/github/kubernetes-sigs/kustomize/release/kustomize%2Fv5.4.1?utm_source=openai
- 2: https://newreleases.io/project/github/kubernetes-sigs/kustomize/release/kustomize%2Fv5.5.0?utm_source=openai
- 3: https://github.com/kubernetes-sigs/kustomize/releases?utm_source=openai
- 4: https://newreleases.io/project/github/kubernetes-sigs/kustomize/release/kustomize%2Fv5.4.1?utm_source=openai
- 5: https://newreleases.io/project/github/kubernetes-sigs/kustomize/release/kustomize%2Fv5.5.0?utm_source=openai
- 6: https://github.com/kubernetes-sigs/kustomize/releases?utm_source=openai
🌐 Web query:
sigs.k8s.io/kustomize/kustomize/v5 v5.7.1 changelog
💡 Result:
Summary of kustomize v5.7.1 (sigs.k8s.io/kustomize/kustomize/v5)
- Drops the shlex dependency (replace shlex-based arg parsing for exec plugin).
- Updates dependencies: kyaml → v0.20.1, cmd/config → v0.20.1, api → v0.20.1.
- Note: release message warns to file an issue if existing manifests become corrupted by the exec-plugin arg parsing change.
Source: GitHub release for kustomize v5.7.1. [1]
[1] kubernetes-sigs/kustomize — Releases (kustomize/v5.7.1).
Kustomize v5.5.0 removed Starlark support for KRM functions and v5.7.1 changed exec plugin argument parsing due to dropping the shlex dependency.
The update path (v5.4.1 → v5.7.1) introduces breaking changes in v5.5.0 (Starlark support removal) and a behavioral change in v5.7.1 (shlex dependency drop affecting exec plugin arg parsing). Verify whether:
- Your kustomize configurations or plugins use Starlark for KRM functions
- Your workflows rely on the exec plugin—if so, test argument parsing after upgrading
🤖 Prompt for AI Agents
In @hack/tools/go.mod at line 11, Your updated dependency line
"sigs.k8s.io/kustomize/kustomize/v5 v5.7.1" introduces breaking changes: v5.5.0
removed Starlark KRM function support and v5.7.1 changed exec plugin arg
parsing; inspect any use of Starlark-based KRM functions and any exec plugins in
your kustomize overlays/plugins, run end-to-end builds to validate behavior, and
either (a) adapt those plugins/manifests to native Go functions or alternative
scripting or adjust exec plugin invocation to the new argument parsing, or (b)
pin the kustomize version to a compatible release (e.g., v5.4.1) in the go.mod
until you’ve migrated and tested.
|
@vr4manta: This pull request references SPLAT-2167 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
go.mod (1)
318-320: Remove or clarify the commented-out replace directive.The comment on line 319 states "Using replace for force update" but the replace directive on line 320 is commented out, making it inactive and misleading. Additionally, the commented replace would be a no-op (replacing the module with itself at the same version).
Consider:
- Removing these lines if they're leftover development notes, or
- Clarifying the documentation if this is meant as a reference for future maintainers.
🧹 Suggested cleanup
- -// Using replace for force update of 2.10 -//replace sigs.k8s.io/cluster-api-provider-aws/v2 => sigs.k8s.io/cluster-api-provider-aws/v2 v2.10.1-0.20251223195230-7aa98eaf0ba3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (44)
e2e/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumhack/tools/go.sumis excluded by!**/*.sumvendor/github.com/sergi/go-diff/AUTHORSis excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/CONTRIBUTORSis excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/diff.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/diffmatchpatch.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/match.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/mathutil.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/operation_string.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/patch.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/stringutil.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/awsmachine_types.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/awsmachine_webhook.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/awsmachinetemplate_webhook.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/types.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/cmd/config/internal/commands/grep.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/cmd/config/internal/commands/run-fns.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/cmd/config/internal/generateddocs/api/docs.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/build.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/flagenablehelm.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/flagsforfunctions.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/create/create.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addbase.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addbuildmetadata.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addcomponent.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addgenerator.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addmetadata.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addresource.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addtransformer.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/fix/convert.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removebuildmetadata.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removeconfigmap.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removemetadata.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removeresource.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removesecret.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removetransformer.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/internal/kustfile/kustomizationfile.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/internal/util/validate.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/localize/localize.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kyaml/copyutil/copyutil.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (3)
e2e/go.modgo.modhack/tools/go.mod
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/go.mod
- hack/tools/go.mod
🔇 Additional comments (1)
go.mod (1)
41-41: This dependency bump correctly pulls in the upstream host affinity fix.The pseudo-version points to commit 7aa98eaf0ba3 (full SHA: 7aa98eaf0ba3d6436ef17db0cd1b7a65b40bc5a9), which is the merge of PR #5801 to the release-2.10 branch. The fix changes the HostAffinity default from
hosttodefaultand improves API documentation, as intended.
|
@vr4manta: This pull request references SPLAT-2167 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
go.mod (1)
318-320: Remove or clarify the dead commented code.The commented-out
replacedirective on line 320 is redundant since line 41 already requires the same pseudo-version directly. The comment on line 319 ("Using replace for force update of 2.10") is misleading because no replace is actually in effect.Either remove these lines entirely or, if kept for documentation purposes, update the comment to clarify this is historical context.
🧹 Suggested cleanup
- -// Using replace for force update of 2.10 -//replace sigs.k8s.io/cluster-api-provider-aws/v2 => sigs.k8s.io/cluster-api-provider-aws/v2 v2.10.1-0.20251223195230-7aa98eaf0ba3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (44)
e2e/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumhack/tools/go.sumis excluded by!**/*.sumvendor/github.com/sergi/go-diff/AUTHORSis excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/CONTRIBUTORSis excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/diff.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/diffmatchpatch.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/match.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/mathutil.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/operation_string.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/patch.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/stringutil.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/awsmachine_types.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/awsmachine_webhook.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/awsmachinetemplate_webhook.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/types.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/cmd/config/internal/commands/grep.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/cmd/config/internal/commands/run-fns.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/cmd/config/internal/generateddocs/api/docs.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/build.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/flagenablehelm.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/flagsforfunctions.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/create/create.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addbase.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addbuildmetadata.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addcomponent.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addgenerator.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addmetadata.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addresource.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addtransformer.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/fix/convert.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removebuildmetadata.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removeconfigmap.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removemetadata.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removeresource.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removesecret.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removetransformer.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/internal/kustfile/kustomizationfile.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/internal/util/validate.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/localize/localize.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kyaml/copyutil/copyutil.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (8)
e2e/go.modgo.modhack/tools/go.modmanifests/0000_30_cluster-api_01_credentials-request.yamlpkg/conversion/capi2mapi/aws_fuzz_test.gopkg/conversion/capi2mapi/aws_test.gopkg/conversion/mapi2capi/aws_fuzz_test.gopkg/conversion/mapi2capi/aws_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- hack/tools/go.mod
- e2e/go.mod
- manifests/0000_30_cluster-api_01_credentials-request.yaml
🔇 Additional comments (5)
pkg/conversion/mapi2capi/aws_test.go (1)
96-121: LGTM!The new test entries properly cover the two main HostPlacement scenarios for MAPI→CAPI conversion:
AnyAvailableaffinity without a dedicated host IDDedicatedHostaffinity with a valid host IDThe host ID format
h-0123456789abcdef0correctly follows the expected pattern (h- prefix + 17 lowercase hex characters).pkg/conversion/capi2mapi/aws_fuzz_test.go (1)
196-223: LGTM!The refactored
fuzzAWSMachineSpecTenancyfunction now correctly operates on the fullAWSMachineSpec, ensuring that correlated fields (Tenancy,HostAffinity,HostID) are set consistently for each fuzzing case. The six cases provide good coverage of the valid combinations:
- Default and dedicated tenancy without host placement
- Host tenancy with
defaultaffinity (any available host) with/without a host ID- Host tenancy with
hostaffinity (dedicated host) requiring a host ID- Empty tenancy baseline
pkg/conversion/capi2mapi/aws_test.go (1)
74-214: LGTM!Excellent test coverage for the CAPI→MAPI host placement conversion. The tests comprehensively validate:
- Valid scenarios: Both 8-character and 17-character HostID formats with different HostAffinity values
- Invalid scenarios: Various malformed HostIDs (wrong length, uppercase, missing prefix, non-hex characters)
- Edge cases: Missing required HostID when affinity is "host", unsupported affinity values
The distinction in error messages between "Required value" (line 190, when HostID is mandatory) and "Invalid value" (line 201, when HostID is optional but malformed) correctly reflects the different validation contexts.
pkg/conversion/mapi2capi/aws_fuzz_test.go (2)
190-226: LGTM!The
fuzzPlacementfunction correctly covers all six host placement variants for MAPI→CAPI fuzzing, mirroring the structure incapi2mapi/aws_fuzz_test.go. The cases properly maintain consistency between correlated fields (Tenancy,Host.Affinity,Host.DedicatedHost).
310-313: LGTM!The
fuzzPlacementfunction is correctly added to the fuzzer function slice, positioned beforefuzzProviderConfigto ensure placement fields are fuzzed before the provider config processes them.
|
@vr4manta: This pull request references SPLAT-2167 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @go.mod:
- Line 41: The go.mod entry for sigs.k8s.io/cluster-api-provider-aws/v2
currently points to a non-existent pseudo-version
"v2.10.1-0.20251223195230-7aa98eaf0ba3"; update this dependency to a resolvable
version by either changing it to the latest existing release (for example
"v2.10.0") or replacing the pseudo-version with a correct commit/pseudo-version
that actually exists upstream, and if you need the fix from PR #5801 verify
which released tag contains that commit before pinning the version.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (44)
e2e/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumhack/tools/go.sumis excluded by!**/*.sumvendor/github.com/sergi/go-diff/AUTHORSis excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/CONTRIBUTORSis excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/diff.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/diffmatchpatch.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/match.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/mathutil.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/operation_string.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/patch.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sergi/go-diff/diffmatchpatch/stringutil.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/awsmachine_types.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/awsmachine_webhook.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/awsmachinetemplate_webhook.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2/types.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/cmd/config/internal/commands/grep.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/cmd/config/internal/commands/run-fns.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/cmd/config/internal/generateddocs/api/docs.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/build.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/flagenablehelm.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/flagsforfunctions.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/create/create.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addbase.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addbuildmetadata.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addcomponent.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addgenerator.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addmetadata.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addresource.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addtransformer.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/fix/convert.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removebuildmetadata.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removeconfigmap.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removemetadata.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removeresource.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removesecret.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removetransformer.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/internal/kustfile/kustomizationfile.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/internal/util/validate.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/localize/localize.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/kustomize/kyaml/copyutil/copyutil.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (3)
e2e/go.modgo.modhack/tools/go.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/go.mod
🔇 Additional comments (1)
hack/tools/go.mod (1)
11-11: Verify kustomize version update for breaking changes before applying.The proposed kustomize updates introduce confirmed breaking changes:
- v5.5.0: Removed Starlark support for KRM functions—verify your manifests don't depend on this feature.
- v5.7.1: Replaced shlex dependency with carapace shlex, affecting exec-plugin argument parsing—test your exec-plugin invocations if used.
Confirm these changes won't impact your build or development workflows before merging.
damdo
left a comment
damdo
left a comment
There was a problem hiding this comment.
/lgtm
To trigger the tests
/hold
|
Scheduling tests matching the |
|
@vr4manta: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
It seems this project is not responsible for the CRDs of AWS CAPI provider. We'll need to update https://github.com/openshift/cluster-api-provider-aws to pull in the latest changes from upstream that fixes the default value of hostAffinity to be default instead of host. |
|
Thank you @vr4manta , I checked @damdo 's pr kubernetes-sigs/cluster-api-provider-aws#5801 seems only set |
do we log the resulting values? I think its creating "" versions of the field which makes it not != null. I can add a check to ignore "" as well in that case. I'll also take a deeper look to see what else we can do to fix the conversions. |
|
Thank you @vr4manta I created a bug https://issues.redhat.com/browse/OCPBUGS-73821 to better trace the issue and attached the details on that. |
|
@vr4manta I noticed you submitted a PR to modify MAPI. However, I'm wondering if we should modify CAPI instead? Because configuring I also tested this directly with AWS, and setting only Affinity=default is not allowed: This confirms that Affinity requires Tenancy=host to be set as well. We should align with AWS's behavior, shouldn't we? |
I agree this should be fixed upstream. The other PR is to make the two match since its broke now. It will take sprints / months to get this backported into the openshift/cluster-api-provider-aws. Once fixed, we can make match so there isn't breakage this this lasting during development. But I'll discuss with @damdo what options we have and i'll update this PR with the results. |
|
@vr4manta: This pull request references SPLAT-2167 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Created upstream PR to fix CAPA to match AWS cli. |
9 participants
SPLAT-2167
Changes
ec2:DescribeInstanceTypesto cluster api credentials requestDependencies
Notes
There seems to be a required permission that was missing for dedicated host support. When running these changes, a warning event was observed in the cluster capi operator namespace.
Summary by CodeRabbit
New Features
Security / Permissions
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.